drivers: spi_nrfx_spim: Fixes related to 32 Mbps speed on nRF5340 #37632
Conversation
anangl
added
bug
drivers/spi/spi_nrfx_spim.c
Outdated
There was a problem hiding this comment.
What if someone wants to use H1H0 on slower speeds?
There was a problem hiding this comment.
I do have this open #34854, but it may need to be updated to do the init at this location instead of at driver init.
There was a problem hiding this comment.
What if someone wants to use H1H0 on slower speeds?
It is not supported now by the driver, so I actually don't know what I could do about it in this PR. Do you have any suggestion?
There was a problem hiding this comment.
The Kconfig option works for me, alternatively this could check the status of the pins and force them to H1H0 if they are not H1H0 but if the speed is under 32MHz then it does not change the status to S1S0 and leaves it at what it is (either S1S0 or H1H0)
There was a problem hiding this comment.
somewhat related: #37572 (pinctrl could allow to set arbitrary pin settings from DT).
drivers/spi/spi_nrfx_spim.c
Outdated
There was a problem hiding this comment.
Just realised that the HAL drivers already perform this configuration in nrfx_spim_init
There was a problem hiding this comment.
I'm sorry that it took me so long to get back to this one. I reworked the PR to take advantage of the pin configuration performed by the nrfx driver. Please take a look once again.
According to the nRF5340 PS, SPIM4 only supports 32 Mbps when the application core is running at 128 MHz. This patch adds the corresponding check. Signed-off-by: Andrzej Głąbek <[email protected]>
According to the nRF5340 PS, for 32 Mbps high-speed SPI using SPIM4, drive configuration H0H1 must be used. The underlying nrfx_spim driver does it properly in its initialization function, so change the shim to (re)initialize the driver when the SPI configuration is to be changed (only then the speed to use is known), to avoid the need of duplicating the corresponding code in the shim itself. Signed-off-by: Andrzej Głąbek <[email protected]>
anangl
force-pushed
the
fix_max_spim_freq_on_nrf5340
branch
from
b618963 to
60e3d95
Compare
|
Rebased. |
anangl
force-pushed
the
fix_max_spim_freq_on_nrf5340
branch
from
60e3d95 to
73c8c5c
Compare
Apply the same changes as the previous commit made in the spi_nrfx_spim shim, to keep these two shims aligned. Signed-off-by: Andrzej Głąbek <[email protected]>
anangl
force-pushed
the
fix_max_spim_freq_on_nrf5340
branch
from
73c8c5c to
4e1cf36
Compare
|
@JordanYates could you please take another look? |
|
@gmarull can you approve if happy please? |
gmarull
left a comment
gmarull
left a comment
There was a problem hiding this comment.
I only have one comment regarding PM.
| /* No action needed at this point, nrfx_spim_init() will be | ||
| * called at configuration before the next transfer. | ||
| */ |
There was a problem hiding this comment.
PM hooks are no longer doing what is expected, ie, put the device into operational state when resume is called. Why is this change required?
There was a problem hiding this comment.
Previously, when resume was called, the device was brought to the state it was right after its initialization - it still needed to be reconfigured before the next transfer:
zephyr/drivers/spi/spi_nrfx_spim.c
Lines 336 to 338 in 7ccc1a4
(at the device initialization, the nrfx driver was initialized with a default configuration, which was then altered accordingly when a transfer was requested). Now the difference is that the initialization of the device does not include the initialization of the nrfx driver - this is done when the device is configured right before a transfer is performed (only then we know the frequency, SPI mode, etc. that should be used), so there is no point in calling
nrfx_spim_init() here and then, right before the next transfer, calling nrfx_spim_uninit() in order to be able to call nrfx_spim_init() again.Alternatively, I would need to store the currently used nrfx driver configuration to be able to use it here in the call to
nrfx_spim_init(). Then it would be not needed to reconfigure the device before the next transfer, provided that transfer would be requested with the same configuration as the last transfer before the device was suspended. But I'm not sure if it is worth spending more RAM just to achieve this possibility.
| data->ctx.config = NULL; | ||
| /* No action needed at this point, nrfx_spi_init() will be | ||
| * called at configuration before the next transfer. | ||
| */ |
|
@JordanYates please take another look |
8 participants
According to the nRF5340 PS, SPIM4 only supports 32 Mbps when the application core is running at 128 MHz. Drive configuration H0H1 must also be used for its pins for this speed.
Fixes #37221.